You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PR for introducing a fix to issue #357: Sync feature not handling non-existing local routes present remotely
Added robustness checks for empty local or remote routes
Added possibility to pass empty list of routes at startup to sync with just remote routes
Fixed the bug causing not handling of non-existing local routes present remotely
Can be tested with basic code like the following and playing with local and remote routes:
from semantic_router import Route
from semantic_router.encoders import OpenAIEncoder
from semantic_router.index.pinecone import PineconeIndex
from semantic_router.layer import RouteLayer
politics = Route(
name="politics",
utterances=[
"isn't politics the best thing ever",
"why don't you tell me about your political opinions",
"don't you just love the president",
],
)
chitchat = Route(
name="chitchat",
utterances=[
"how's the weather today?",
"how are things going?",
],
)
routes = [politics, chitchat]
encoder = OpenAIEncoder(openai_api_key=openai_api_key)
pc_index = PineconeIndex(
api_key=pinecone_api_key,
region="us-east-1",
index_name="alai-clearwater-routes-input-intent",
sync="local",
)
rl = RouteLayer(encoder=encoder, routes=routes, index=pc_index)
PR Type
Bug fix, Enhancement
Description
Added robustness checks for empty local or remote routes during synchronization.
Enhanced _sync_index method to handle various synchronization modes more effectively.
Modified _add_and_sync method to return the updated list of Route objects.
Introduced _set_layer_routes method in RouteLayer to set and override current routes.
Updated RouteLayer initialization to handle cases where no local routes are provided and sync is set to use remote routes.
Changes walkthrough 📝
Relevant files
Enhancement
pinecone.py
Improve sync feature robustness and handling of routes
semantic_router/index/pinecone.py
Added checks for empty local or remote routes during sync.
Enhanced _sync_index to handle various sync modes more robustly.
Modified _add_and_sync to return updated list of Route objects.
Updated synchronization logic to include layer_routes.
Error Handling The error handling in _sync_index might be too strict or unclear for users. The multiple nested conditions and checks for route presence could be simplified or better documented to enhance maintainability and readability.
Method Complexity The _sync_index method has become significantly complex with multiple responsibilities, including error handling, route synchronization, and updating route lists. Consider refactoring to separate concerns and simplify the logic.
Return Type Change The method _add_and_sync has an updated return type to include a list of Route objects, which might affect existing integrations or require updates in the calling code.
Initialization Logic The initialization logic in RouteLayer now handles cases where no routes are provided differently based on the sync mode. This change could lead to unexpected behaviors if not properly documented or understood.
Specify which condition (local or remote) caused the error in the error message for better clarity
The error message "No routes found in the index." is raised in multiple conditions. It would be more informative to specify which condition (local or remote) caused the error to help with debugging.
-raise ValueError("No routes found in the index.")+if not remote_routes:+ raise ValueError("No remote routes found in the index.")+if not local_routes["routes"]:+ raise ValueError("No local routes found in the index.")
Suggestion importance[1-10]: 9
Why: This suggestion enhances error messages, making debugging easier by specifying whether the local or remote condition caused the error. It significantly improves code clarity and usability.
9
Best practice
Initialize the layer_routes dictionary at the start of the function to ensure it is always defined
To ensure that the layer_routes dictionary is consistently initialized, it should be defined outside of the conditional blocks. This avoids potential NameError if none of the conditions are met before its usage.
layer_routes = {}
+# Initialize layer_routes at the start of the function to ensure it is always defined.
Suggestion importance[1-10]: 8
Why: This suggestion ensures that the layer_routes dictionary is always defined, preventing potential NameError issues. It addresses a potential bug and improves code reliability.
8
Maintainability
Refactor repeated sync setting checks into a separate method to simplify the _sync_index method
The conditional checks for sync settings are repeated multiple times. Consider refactoring these checks into a separate method to simplify the _sync_index method and improve code maintainability.
Why: This suggestion improves code maintainability by reducing repetition and simplifying the _sync_index method. However, it introduces additional methods which may add slight overhead.
7
Break down the _sync_index method into smaller methods to improve readability and maintainability
The method _sync_index is complex due to multiple nested conditions. Consider breaking down this method into smaller, more focused methods to improve readability and maintainability.
Why: This suggestion improves readability and maintainability by breaking down a complex method into smaller, focused methods. However, it requires careful implementation to ensure functionality remains intact.
6
Vits-99
changed the title
Fixed issue #357 Sync feature not handling non-existing local routes present remotely
Fixes #357 Sync feature not handling non-existing local routes present remotely
Jul 16, 2024
jamescalam
changed the title
Fixes #357 Sync feature not handling non-existing local routes present remotely
fix: Sync feature not handling non-existing local routes present remotely
Jul 17, 2024
jamescalam
changed the title
fix: Sync feature not handling non-existing local routes present remotely
fix: handling non-existing local routes in sync
Jul 31, 2024
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
PR for introducing a fix to issue #357: Sync feature not handling non-existing local routes present remotely
Can be tested with basic code like the following and playing with local and remote routes:
PR Type
Bug fix, Enhancement
Description
_sync_index
method to handle various synchronization modes more effectively._add_and_sync
method to return the updated list ofRoute
objects._set_layer_routes
method inRouteLayer
to set and override current routes.RouteLayer
initialization to handle cases where no local routes are provided and sync is set to use remote routes.Changes walkthrough 📝
pinecone.py
Improve sync feature robustness and handling of routes
semantic_router/index/pinecone.py
_sync_index
to handle various sync modes more robustly._add_and_sync
to return updated list ofRoute
objects.layer_routes
.layer.py
Initialize index with remote routes and manage layer routes
semantic_router/layer.py
empty.
_set_layer_routes
method to set and override currentroutes.
_add_routes
to use the new sync logic and set layer routes.